Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce calling scp, cache result #3025

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Reduce calling scp, cache result #3025

wants to merge 3 commits into from

Conversation

DJ3CE
Copy link
Contributor

@DJ3CE DJ3CE commented Mar 24, 2024

For callsign suggestions in logging, Cloudlog requested a list from the server with every additional character typed, from a minimum of 3 on.

This PR reduces calls to the server, by saving and filtering a (matching) list from the server for additional characters typed.

Example: You typed 'DJ3' and got a list of all callsigns. Now you add 'C' ('DJ3C') and while previously, the server was asked to give a new suggestion list with this PR the already received list is filtered for 'DJ3C'.

If the original request does not match the typed callsign anymore (Example: You replace the '3' in the previous example by a '4'), a new request is made.

Included in this PR is contest-, as well as normal-logging.

@patrickrb
Copy link
Contributor

patrickrb commented Mar 27, 2024

I think just adding a debounce might be a better solution:

// Debounce function
function debounce(func, wait) {
    var timeout;
    return function() {
        var context = this, args = arguments;
        clearTimeout(timeout);
        timeout = setTimeout(function() {
            func.apply(context, args);
        }, wait);
    };
}

// On Key up check and suggest callsigns
$("#callsign").keyup(debounce(function () {
    var call = $(this).val();
    if (call.length >= 3) {

        $.ajax({
            url: 'lookup/scp',
            method: 'POST',
            data: {
                callsign: $(this).val().toUpperCase()
            },
            success: function (result) {
                $('.callsign-suggestions').text(result);
                highlight(call.toUpperCase());
            }
        });
        // moved to blur
        // checkIfWorkedBefore();
        var qTable = $('.qsotable').DataTable();
        qTable.search(call).draw();
    }
    else if (call.length <= 2) {
        $('.callsign-suggestions').text("");
    }
}, 500)); // 500ms delay

not sure if this pseudo code works, but the idea is to just wait some delay for the user to stop typing before firing off a request

Tested it locally and it works, just not sure what the best delay amount would be, this was a test w/ a 3 second delay just to ensure the code is working..itd probably be better at like ~1 second also we should add a spinner next to the input so the user knows theres a request being sent:

3-26-2024 (21-31-33)

@DJ3CE
Copy link
Contributor Author

DJ3CE commented Mar 27, 2024

Not sure if we want to solve the same problem ;). I'd go for trying to get suggestions as early and fast as possible and narrow them down as fast as possible, therefore it seems counterintuitive to do 'debouncing', but more natural to filter stored results.

@patrickrb
Copy link
Contributor

patrickrb commented Mar 27, 2024

So based on the pr description, I thought we were trying to reduce the number of requests sent from the frontend to the backend as the user types.

For callsign suggestions in logging, Cloudlog requested a list from the server with every additional character typed, from a minimum of 3 on.
This PR reduces calls to the server,

The debounce solution is the standard go to way of preventing sending a request as a user types until they are done typing.

I think implementing caching here is setting up for problems down the road.

There's an old joke in programming:

There are only two hard problems in computer science: Cache invalidation, naming things, and off-by-one errors.

@DJ3CE
Copy link
Contributor Author

DJ3CE commented Mar 27, 2024

... but the cited sentence has some more words to it ;).

I'm totally fine with alternative solutions taking other priorities in server-load and user-experience, but I guess here I'd appreciate an eye on this particular approach - especially, as pointed out, since cache invalidation is not the easiest thing to do. Feel free to point out gaps I might have left.

The approach taken just uses a state-model representing if and for which characters a request is on its way.

@DJ3CE
Copy link
Contributor Author

DJ3CE commented Mar 27, 2024

... one thing to add: I guess there could be different solutions 'right' for 'normal' and for 'contest' logging.

Based on your screenshots you seem to focus on 'normal'-logging, while I started implementing this for contest-logging.

@patrickrb
Copy link
Contributor

patrickrb commented Mar 27, 2024

I definitely see where you're coming from, and caching is a valid way of solving the stated problem.

Both solutions solve the problem, and can even work in tandem together.

With that said, in my personal experience, it would best to start with the debounce method at ~300-500ms delay (which isn't even noticeable to the end user, it's just enough to prevent multiple calls if the user mistypes and quickly fixes).

The main reasons to start with debounce over cache are:

  1. Ease of Implementation: Debouncing can be implemented quickly, This makes it a low-effort, high-impact change.
  2. Minimal Risk: Since debouncing is applied on the frontend and primarily affects how often the application makes requests to the backend, it introduces minimal risk to existing backend logic or data integrity.

If we implement debounce and *you're still not happy with how it solves the problem, then I think it'd be the appropriate time to look at integrating some type of caching.

I hope this clears up where I'm coming from and the best practice for order of operations on a problem/solution like this.

@magicbug
Copy link
Owner

For SCP you want to get results as fast as possible this obviously is different for the QSO box fields some debounce on those would be a great idea.

@magicbug
Copy link
Owner

This PR does seem to have an issue for example with my callsign when I type 2M0 no SCP results ever appear but GM1 etc function as expected

assets/js/sections/contesting.js Outdated Show resolved Hide resolved
assets/js/sections/contesting.js Outdated Show resolved Hide resolved
assets/js/sections/contesting.js Outdated Show resolved Hide resolved
assets/js/sections/contesting.js Outdated Show resolved Hide resolved
assets/js/sections/contesting.js Outdated Show resolved Hide resolved
assets/js/sections/qso.js Outdated Show resolved Hide resolved
assets/js/sections/qso.js Outdated Show resolved Hide resolved
@DJ3CE
Copy link
Contributor Author

DJ3CE commented Mar 29, 2024

This PR does seem to have an issue for example with my callsign when I type 2M0 no SCP results ever appear but GM1 etc function as expected

Thanks for trying this out and pointing me to issues where '0' is part of the callsign. What is the convention about the use of '0' or 'Ø' in callsigns in Cloudlog?

- fix '0'
- improvements in data handling
- filter allows prefixes
@DJ3CE
Copy link
Contributor Author

DJ3CE commented Mar 29, 2024

There you go. Thank you for input and testing, appreciate it.

  • @magicbug callsigns including '0' now work :). The returned list hast stroked-0 in it, thus the filtering failed (miserably ;) ),
  • instead of String.includes() a regex is used, to have prefixed callsigns in the list while not being too coarse,
  • scp_data.status got discarded,
  • 'rough' network conditions are handled better by asking if a server-response still matches the currently entered part-of-a-callsign

@DJ3CE
Copy link
Contributor Author

DJ3CE commented Apr 1, 2024

@magicbug, please let me know if there's anything which prevents merging.

Copy link
Contributor

@patrickrb patrickrb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still dont like this change, but i can add more of my changes if this get's accepted.

assets/js/sections/contesting.js Outdated Show resolved Hide resolved
assets/js/sections/contesting.js Outdated Show resolved Hide resolved
assets/js/sections/contesting.js Outdated Show resolved Hide resolved
assets/js/sections/qso.js Outdated Show resolved Hide resolved
assets/js/sections/qso.js Outdated Show resolved Hide resolved
Copy link
Contributor

@patrickrb patrickrb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In JavaScript, whether to use let, const, or var for variable declarations depends on the specific use case and the scope where the variable is needed.

var: This is the oldest keyword for declaring variables, existing since the beginning of JavaScript. Variables declared with var are function-scoped or globally-scoped if declared outside of a function. They can be re-declared and updated. However, var has some pitfalls, such as hoisting, which can lead to bugs, especially in large codebases.

let: Introduced in ES6 (ECMAScript 2015), let provides block-scoped variables. Unlike var, variables declared with let are limited in scope to the block, statement, or expression where they are used. let allows you to update the variable but not re-declare it within the same scope. This reduces the risk of bugs compared to var.

const: Also introduced in ES6, const is used to declare variables whose values are not intended to change. const provides block-scoping similar to let. The key difference is that once a variable is assigned with const, its value cannot be reassigned. Attempting to do so will result in a runtime error. However, note that if a const variable holds an object or an array, the object or array's contents can still be modified (the variable's reference to the object or array cannot change).

Best Practices
Prefer const: Use const by default for all of your variable declarations if you know their values will not change. This makes your code easier to reason about since you know these variables won't be reassigned later in your code.

Use let for variables that change: If you have variables that need to change, like counters in a loop, or values that get reassigned, use let. This signals to others that this variable will be reassigned.

Avoid var: Due to its function scope and hoisting behavior, var can lead to confusion and bugs, especially in complex codebases. Modern JavaScript codebases tend to avoid var in favor of let and const for clearer scoping rules.

In conclusion, while it's not a rule to "always use let," the modern practice leans towards using const by default and let when necessary for variables whose values need to change. Avoiding var is generally considered good practice in contemporary JavaScript development.

@@ -8,6 +8,11 @@ $(document).ready(async function () {
setRst($("#mode").val());
});

var scp_data = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be let, i will net let it go.

highlight(call.toUpperCase());
}
});
if ( scp_data.request == "" || ! call.startsWith(scp_data.request) ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be ===

callsign: call
},
success: function (result) {
var call_now = $("#callsign").val().toUpperCase(); // Might have changed while running the query...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should actually be let,

@@ -1,4 +1,9 @@
var lastCallsignUpdated=""
var scp_data = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another let

@patrickrb
Copy link
Contributor

Please read up on javascript best practices before refusing changes that catch the code up to javascript best practices.

We should ALWAYS use === checks.

We should ALWAYS use let and const, NEVER var.

@patrickrb
Copy link
Contributor

patrickrb commented Apr 1, 2024

Feel free not to use the .abort logic if you think its too complex, but the rest of the keyUp function should look like this:

// On Key up check and suggest callsigns
$("#callsign").keyup(() => {
	const call = $(this).val().toUpperCase();
	const cleanedCall = call.replace('0', 'Ø');

	// Exit early if the users starts typing a completely new call
	if (scp_data.previousRequest && !call.startsWith(scp_data.request)) {
		scp_data.previousRequest.abort();
	}

	if (call.length >= 3) {
		if (scp_data.request === "" || !call.startsWith(scp_data.request)) {
			scp_data.request = call;
			scp_data.data = [];
			scp_data.previousRequest = $.ajax({
				url: 'lookup/scp',
				method: 'POST',
				data: {
					callsign: call
				},
				success: function (result) {
					const currentCall = $("#callsign").val().replace('0', 'Ø').toUpperCase();
					if (currentCall.startsWith(call)) {
						scp_data.data = result.split(" ");
						$('.callsign-suggestions').text(filterCallsignList(currentCall, scp_data.data));
						highlight(currentCall);
					}
				},
				complete: function () {
					scp_data.previousRequest = null;
				}
			});

		} else {
			// Filter the already obtained list:
			$('.callsign-suggestions').text(filterCallsignList(cleanedCall, scp_data.data));
			highlight(cleanedCall);
		}
		const qTable = $('.qsotable').DataTable();
		qTable.search(call).draw();
	}
	else {
		$('.callsign-suggestions').text("");
	}
});

It's inarguably cleaner than what've you've provided in your PR.

@magicbug
Copy link
Owner

magicbug commented Apr 2, 2024

This PR does seem to have an issue for example with my callsign when I type 2M0 no SCP results ever appear but GM1 etc function as expected

Thanks for trying this out and pointing me to issues where '0' is part of the callsign. What is the convention about the use of '0' or 'Ø' in callsigns in Cloudlog?

I'll be honest the convention in Cloudlog should be Ø but.. it varies perhaps something for improvement.

Can confirm that it now works, but we have lost the ability to just type the suffix and show matches based on that which is super handy with the SCP box

- server side matches anywhere, thus replace `startsWith` by `includes`,
- callsigns *should* be with stroked-0, improving consistency here,
- code restructuring (reduce nesting a lot),
- isolating code, same codebase for qso.js and contesting.js, could
  be put in a 'lib'-like file
@DJ3CE
Copy link
Contributor Author

DJ3CE commented Apr 4, 2024

I'll be honest the convention in Cloudlog should be Ø ...

Sounds like s.th. to work with and to have incremental improvement :).

Now replacing 'Ø' by '0' in controllers/Lookup.php:scp(), as both scp-files (clublog_scp.txt and masterscp.txt) and the db-table use '0', then it makes no difference how the backend gets called. The conversion back to 'Ø' is already done in scp(), an exception of '0's shows up with getSessionQsos in contesting.

Can confirm that it now works, but we have lost the ability to just type the suffix and show matches based on that which is super handy with the SCP box

Thanks for pointing that out, haven't observed nor used that up to now (I seem not to remember suffixes without prefixes :D), so now consistently switching to String.includes() for matching cache-validity and showing results. @patrickrb already mentioned includes(), but neither with the background @magicbug gave here nor consistent - sorry, haven't seen that before.

Is there a library-like js-file to have one place for code(-pieces) like this? I have separated the 'functional' code from its specific application to a keyup, so both, qso.js and contesting.js have the same functional code, which could be put in a single place elsewhere (... and no more global-variable is needed :) ).

Edit: I might add, that the check for cache validity got inverted, you'll directly get to the updated expression with de Morgan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants